-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
storage: fix deadlock in consistency queue #25456
Conversation
Review status: 0 of 1 files reviewed at latest revision, all discussions resolved, some commit checks failed. pkg/storage/consistency_queue.go, line 107 at r1 (raw file):
Isn't this too soon? Comments from Reviewable |
8474c5c
to
4c13f6d
Compare
Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion. pkg/storage/consistency_queue.go, line 107 at r1 (raw file): Previously, bdarnell (Ben Darnell) wrote…
Good point, but there was an even bigger problem: I didn't actually give the pkg/storage/replica_test.go, line 9270 at r2 (raw file):
I put this here so that it's less likely to conflict when backporting (and it can't be in the proper file because that is in storage_test). Comments from Reviewable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just nits. Thanks for tracking this down.
pkg/storage/consistency_queue.go
Outdated
|
||
// Update the last processed time for this queue. We do this before actually | ||
// processing because this queue will do it even in the case of an error | ||
// (since a failed processing run could be caused by down followers). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear from the comment why this is worth doing specifically for the consistency queue but not for any other queues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really just backfilling rationale here, but this is the previous behavior. I'm going to add a bit of doubt to my comment to not make it seem like this was a decision prompted by an observable problem.
pkg/storage/replica_test.go
Outdated
@@ -9267,6 +9267,34 @@ func TestReplicaRecomputeStats(t *testing.T) { | |||
} | |||
} | |||
|
|||
// TestConsistenctQueueErrorFromCheckConsistency exercises the case in which |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/Consistenct/Consistency
pkg/storage/replica_test.go
Outdated
@@ -9267,6 +9267,34 @@ func TestReplicaRecomputeStats(t *testing.T) { | |||
} | |||
} | |||
|
|||
// TestConsistenctQueueErrorFromCheckConsistency exercises the case in which | |||
// the queue receives an error from CheckConsistency. | |||
func TestConsistenctQueueErrorFromCheckConsistency(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing a leak-checker call
4c13f6d
to
1fbd0aa
Compare
Reviewed 1 of 2 files at r3. pkg/storage/consistency_queue.go, line 108 at r2 (raw file): Previously, tschottdorf (Tobias Schottdorf) wrote…
The reason we call setQueueLastProcessed in consistencyQueue and timeSeriesMaintenanceQueue is that these queues target a much longer cycle time (24h, compared to 10m for the other queues), so it's more valuable to persist the last processing time. The fact that consistencyQueue does this even on error is, I think, just an accident. Comments from Reviewable |
When `CheckConsistency` returns an error, the queue checks whether the store is draining to decide whether the error is worth logging. Unfortunately this check was incorrect and would block until the store actually started draining. A toy example of this problem is below (this will deadlock). The dual return form of chan receive isn't non-blocking -- the second parameter indicates whether the received value corresponds to a closing of the channel. Switch to a `select` instead. ``` package main import ( "fmt" ) func main() { ch := make(chan struct{}) _, ok := <-ch fmt.Println(ok) } ``` Touches cockroachdb#21824. Release note (bug fix): Prevent the consistency checker from deadlocking. This would previously manifest itself as a steady number of replicas queued for consistency checking on one or more nodes and would resolve by restarting the affected nodes.
1fbd0aa
to
7055cc8
Compare
Updated the comment. bors r=bdarnell,a-robinson |
25297: build: add roachtests to be run on every PR r=benesch,petermattis a=danhhz Release note: None 25434: opt: Change how Any operator is represented and flattened r=andy-kimball a=andy-kimball The Any operator currently takes a single input rowset, and expects it to return a single boolean column. The hoister flattens the Any operator by testing and projecting that boolean column. The problem is that this representation cannot easily be decorrelated. Example: z = ANY(SELECT x FROM xy) This is currently represented as: (Any (Project xy [ z=x ])) The z=x projection field cannot easily be hoisted over a left join. This commit uses an alternate representation: (Any xy z EqOp) The new representation keeps the input, scalar, and comparison op components separate, so they can be combined in ways that it easier to decorrelate. 25456: storage: fix deadlock in consistency queue r=bdarnell,a-robinson a=tschottdorf When `CheckConsistency` returns an error, the queue checks whether the store is draining to decide whether the error is worth logging. Unfortunately this check was incorrect and would block until the store actually started draining. A toy example of this problem is below (this will deadlock). The dual return form of chan receive isn't non-blocking -- the second parameter indicates whether the received value corresponds to a closing of the channel. Switch to a `select` instead. ```go package main import ( "fmt" ) func main() { ch := make(chan struct{}) _, ok := <-ch fmt.Println(ok) } ``` Touches #21824. Release note (bug fix): Prevent the consistency checker from deadlocking. This would previously manifest itself as a steady number of replicas queued for consistency checking on one or more nodes and would resolve by restarting the affected nodes. Co-authored-by: Daniel Harrison <[email protected]> Co-authored-by: Andrew Kimball <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]>
Build succeeded |
25471: backport-2.0: rocksdb: use max_manifest_file_size option r=bdarnell a=tschottdorf Backport 1/1 commits from #25341. /cc @cockroachdb/release --- Cockroach uses a single long running rocksdb instance for the entire process lifetime, which could be many months. By default, rocksdb tracks filesystem state changes in a log file called the MANIFEST, which grows without bound until the instance is re-opened. We should bound the maximum file size of rocksdb MANIFEST using the corresponding rocksdb option to prevent unbounded growth. The MANIFEST file grew to several GBs in size in a customer bug report but that was probably because of some other bad behavior in rocksdb state management. We do want to bound the MANIFEST size in such cases as well. Release note: None 25474: backport-2.0: storage: fix deadlock in consistency queue r=bdarnell a=tschottdorf Backport 1/1 commits from #25456. /cc @cockroachdb/release --- When `CheckConsistency` returns an error, the queue checks whether the store is draining to decide whether the error is worth logging. Unfortunately this check was incorrect and would block until the store actually started draining. A toy example of this problem is below (this will deadlock). The dual return form of chan receive isn't non-blocking -- the second parameter indicates whether the received value corresponds to a closing of the channel. Switch to a `select` instead. ```go package main import ( "fmt" ) func main() { ch := make(chan struct{}) _, ok := <-ch fmt.Println(ok) } ``` Touches #21824. Release note (bug fix): Prevent the consistency checker from deadlocking. This would previously manifest itself as a steady number of replicas queued for consistency checking on one or more nodes and would resolve by restarting the affected nodes. Co-authored-by: Garvit Juniwal <[email protected]> Co-authored-by: Tobias Schottdorf <[email protected]>
When
CheckConsistency
returns an error, the queue checks whether thestore is draining to decide whether the error is worth logging.
Unfortunately this check was incorrect and would block until the store
actually started draining.
A toy example of this problem is below (this will deadlock). The dual
return form of chan receive isn't non-blocking -- the second parameter
indicates whether the received value corresponds to a closing of the
channel.
Switch to a
select
instead.Touches #21824.
Release note (bug fix): Prevent the consistency checker from
deadlocking. This would previously manifest itself as a steady number of
replicas queued for consistency checking on one or more nodes and would
resolve by restarting the affected nodes.